Fix Int overflow on 32-bit platforms (wasm32, armv7) - #88

Yes, valid point. The library was built primarily targeting 64-bit platforms but after the cross-platform effort invariably there are still 32-bit targets in this day and age. WASM64 is in the works but likely not viable any time soon.

Forgive my possible naïveté, but Int64 is available in Swift on 32-bit platforms (and cross-compiles successfully on the Swift WASM SDK). Can we not just respell Int as Int64 where necessary without any platform-conditional logic changes?

wasm CI jobs added [...] run swift build only.
If you'd like, I can follow up with a PR that runs swift test on the wasm jobs via wasmtime or Node.

That would actually be fantastic if you would like to. Preferably branch off main and punt it over as a new PR even if the actual tests fail. I've only recently added Android and now WASM build jobs to repository CI pipelines, but just haven't had time to look into how to get actual unit tests happening on CI.

Not naïve at all — you're right, and it's the better fix. I've force-pushed it onto this branch (the saturating version is gone; shout if you'd rather have had it as a separate PR and I'll restore).

No platform-conditional logic anywhere. The internal domain widens cleanly because the sfcNew locals infer their type:

• internal: FrameCount.subFrameCount, framesToSubFrames, subFramesToFrames, FrameCount.init(subFrameCount:base:)
• public: TimecodeFrameRate.maxTotalSubFrames(in:base:), maxSubFrameCountExpressible(in:base:), Timecode.maxSubFrameCountExpressible — three signatures, Int → Int64. That is source-breaking for anyone binding the result to an Int, so say the word if you'd rather stage it behind a deprecated overload.

Deliberately not widened: maxTotalFrames, which peaks at 1_036_800_000 (120 fps over 100 days) and fits a 32-bit Int — along with the frames/subFrames components it bounds. Only the count needs 64 bits.

One narrowing remains, commented at the site: Timecode.rationalValue converts back to Int because Fraction is Int-based, so on a 32-bit platform a timecode beyond ~Int32.max subframes has no representable rational value. That is a pre-existing limit of Fraction rather than of the count. Happy to widen Fraction in a separate PR if you want it, but I did not want to expand this one's blast radius uninvited.

Also correcting something I wrote in the original description: I said the product exceeds Int32.max "for every frame rate". That holds at the 80- and 100-subframe bases, but at .quarterFrames the lower rates still fit. The test asserts across every rate/base pair rather than spot-checking, which is how I noticed.

Verified: full suite 506 tests in 55 suites on macOS, and 551 tests in 128 suites on wasm32 under wasmtime — the latter by pinning this branch into my own project, which was the only way I could actually execute your library's code on wasm. Which leads into the CI follow-up you asked for; PR coming, and it explains why running your suite there is not yet a two-line job.

Unlike the saturating version, this makes .max100Days genuinely usable on 32-bit rather than merely non-trapping.

public: Int → Int64. That is source-breaking for anyone binding the result to an Int

My feeling that there should be consistency with consumed and emitted types concerning total frame counts and total subframe counts across the library's public API surface. Inconsistency may be confusing to the consumer if a frame count is typed as Int in one location but Int64 somewhere else. I realize this increases the necessary deprecation overloads but I think it is worth doing. I haven't had a close look at all possible sites concerned, but thought I would mention it.

You may have found them, but for cleanness and conciseness, deprecations all belong in a respective target's /API Evolution folder, where some can be found already. These files are named based upon the release version in which the deprecation appears, so these could be for release 3.1.4. That version can always be updated prior to release if needed of course.

Fraction is Int

In keeping with the concern for consistency, Fraction should likely be migrated to Int64 for its public API surface with deprecations. If you want to address that in this PR it would probably make more sense since it is closely related and reliant on changes made in this PR.

Agreed on consistency — a frame count typed Int in one place and Int64 in another is worse than either alone, and I will follow the /API Evolution convention with a SwiftTimecodeCore-API-3.1.4.swift for the deprecations. Thanks for pointing at that; I had not spotted it.

One finding worth folding into the scope before anyone starts, which came out of the WASM CI work in #89: there is a third total-count domain in the same family — audio samples. Timecode Samples Tests.swift carries literals like 4_147_200_000 stored into Int. That is 48 kHz × 24 hours, so unlike the frame-count ceiling it overflows a 32-bit Int at the library's ordinary limits, with no hypothetical future frame rate required. If totals are being made consistent, samples probably belong in that set alongside frames, subframes and Fraction.

That makes the job meaningfully larger than this PR, which is why I want to ask rather than assume: would you prefer to merge this one as approved and take the consistency work as a follow-up PR, or hold this one and do it all together?

I am happy either way and will do the work regardless — it is your API and your call on how to stage it. My only reason for raising it is that this PR is already approved and fixes a hard trap on 32-bit, so there may be value in it landing on its own rather than waiting behind a larger refactor. If you would rather have one coherent change, say so and I will fold it all in here, Fraction included.

Unrelated, in case it is useful: Tests (macOS), Tests (macOS - Swift 6.2) and Tests (macCatalyst) are all being cancelled at ~30 minutes here — and on main as well; I checked runs from 08-08, 08-09 and 08-10, which all show the same ~30m cutoff. It looks like a job timeout rather than anything in this diff, but it does leave this PR sitting at UNSTABLE despite your approval, so I thought it worth mentioning.

there is a third total-count domain in the same family — audio samples
samples probably belong in that set

Yes, good point.

[macOS CI tests] are all being cancelled at ~30 minutes

Well aware. GitHub CI has been very unreliable and often the runners and Actions backend cause random test failures and cancellations. It's a constant game of plugging leaks in the dam because the runners are a moving target and their composition never stays static for long.

would you prefer to merge this one as approved and take the consistency work as a follow-up PR, or hold this one and do it all together?

I think we can add it to this PR, as it's closely related in scope. The commit history is enough to allow in-situ rollbacks if needed.

[macOS CI tests] are all being cancelled at ~30 minutes

FYI: I was right (#90). The runners are broken. There's nothing wrong with the package or the CI job itself. GitHub wastes so much of my time chasing false positives it's beyond belief.

Consistency work pushed — frames, subframes, samples and Fraction are now all Int64. Full suite green: 506 tests in 55 suites, including the CMTime bridge.

The rule I applied throughout: totals are Int64, per-component values stay Int.

Widened — FrameCount.Value's .frames/.split/.splitUnitInterval payloads, wholeFrames, maxTotalFrames, maxTotalFramesExpressible, maxTotalSubFrames, maxSubFrameCountExpressible, Timecode.Stride, .frames(_:) and .samples(_:) constructors, samplesValue(sampleRate:), and Fraction.numerator/denominator with its internal arithmetic.

Left as Int — Components' h/m/s/f, FrameCount.subFrames, and FeetAndFrames (24h at 24fps is only ~129,600 feet). These are components, not totals.

A nice side effect: widening Fraction let me delete the narrowing that the first version of this PR had to leave in rationalValue, where a 64-bit subframe count was being squeezed back into an Int.

Two things needing your call

1. Int companion overloads, not deprecated. .samples(_:), .frames(_:) and Fraction.init keep Int alongside Int64. This is not redundancy — Int is Swift's default integer-literal type, so with only Int64 and Double overloads present, .samples(48000 * 2, sampleRate: 48000) becomes ambiguous. I left them undeprecated deliberately: marking them deprecated would emit warnings on ordinary literal use, which seems worse than the inconsistency. Happy to flip them if you disagree.

2. No /API Evolution/SwiftTimecodeCore-API-3.1.4.swift yet, and I want to check the approach before writing one. The deprecation strategy only half-applies here:

• Parameter type changes are already covered by the Int companions above — a caller passing an Int keeps compiling.
• Return type changes cannot be deprecated this way at all. samplesValue(sampleRate:), wholeFrames, maxTotalFrames, Stride, and the Fraction properties changed what they return, and Swift will not let a deprecated shim differ only by return type without making call sites ambiguous. So these are simply source-breaking for anyone binding the result to an Int.

If you want migration cover for those, it needs differently-named accessors — something like samplesIntValue(sampleRate:) marked deprecated — rather than overloads. That is a naming decision I would rather you made than have me invent. Tell me which symbols you want covered and how you would like them named, and I will add the 3.1.4 file.

Worth noting the tests caught real 32-bit problems on the way through, not just type churn: several sample-count literals in Timecode Samples Tests.swift overflow a 32-bit Int outright.

Thanks very much.

Swift will not let a deprecated shim differ only by return type without making call sites ambiguous

Not strictly the case.

func foo() -> Int { 0 }

@_disfavoredOverload
func foo() -> Int64 { 1 }

let x = foo() // infers Int, returns `0`
let y: Int64 = foo() // explicitly Int64, returns `1`

The least breaking solution for the consumer would be to keep Int as the preferred overload where possible. It may be feasible to not have any deprecations but instead offer overloads for all type consuming and emitting sites. In which case, they would not go in an API evolution file but go next to their sister method/properties. The side effect may be that 32-bit platforms may need to imperatively constrain types to 64-bit concrete types where overflows may be likely.

If there is a solution that can create the least disruption for the vast amount of consumers who are all working exclusively on 64-bit platforms that would be ideal. It doesn't make a ton of sense making disruptive changes to serve the needs of a tiny fraction of the consumer base.

I tested @_disfavoredOverload before replying — you're right, it behaves exactly as you describe. A bare call infers Int, an annotated one picks Int64, and internal call sites can force Int64 where they need exactness. My claim that Swift wouldn't allow it was wrong.

But testing it turned up something that changes the shape of the decision, so rather than reply I went and measured both options.

@_disfavoredOverload only covers functions

Properties can't be overloaded by type — computed or stored:

struct F {
    var numerator: Int { 1 }
    @_disfavoredOverload
    var numerator: Int64 { 2 }   // error: invalid redeclaration of 'numerator'
}

So "overloads for all type consuming and emitting sites" can cover the functions and initializers, but not Fraction.numerator/denominator, FrameCount.wholeFrames, the FrameCount.Value enum payloads, or the Stride typealias. Each of those has to pick a single type, and picking Int64 is a source break with no overload escape hatch.

Which made me try the opposite extreme

If consistency can't be achieved without breaking changes somewhere, it's worth knowing what the bug actually costs to fix on its own. Turns out: nothing.

Keeping every public signature exactly as it is, computing the bounds in Int64 internally, and having the public Int accessors clamp rather than trap:

Branch: mansbernhardt:experiment/minimal.

The clamp is safe for the same reason the first version of this PR was: these values are only ever used as an upper bound, and a subframe count that large is itself unrepresentable in a 32-bit Int, so clamping still bounds the entire representable domain. On 64-bit it never engages.

Suggestion

Land the minimal fix to close the 32-bit trap with zero disruption to the 64-bit majority, and treat API consistency as its own deliberate change later — because consistency now unavoidably means choosing types for properties that can't be overloaded, which is an API decision rather than a mechanical refactor, and it deserves to be made on its own terms rather than as a side effect of a bug fix.

That said, this is your library and you've already said you'd like the consistency work here. The full refactor is pushed and green if you'd prefer it — just say which and I'll set the PR to match. I'd rather give you the measurements than argue for one.

Thanks for doing the exploratory work on this.

Properties can't be overloaded by type

If the properties were converted to functions (var foo: Int → func foo() → Int) it could be possible, and could have computed property deprecation proxies for them. But that's just contributing further to an already less than ideal over-arching solution.

having the public Int accessors clamp
Land the minimal fix to close the 32-bit trap with zero disruption to the 64-bit majority

The simplicity of this approach without API changes makes the most sense at this point in time. My only hesitation is having values silently clamp instead of returning actual true values, if that behavior is not obvious at the callsite for consumers.

As just one example, audio samples @ 48KHz overflows Int32 at just 13 hours.

treat API consistency as its own deliberate change later
deserves to be made on its own terms rather than as a side effect of a bug fix

Big-picture, yes - you're right. If we adopt Int64 across public API consistently where appropriate, we can also do it cleanly without deprecations or overloads if it is considered a major version bump.

There is one other possibility I might entertain at this junction before we ratify a solution. It wouldn't be entirely out of form to conditionally substitute Int64 using compiler fences only on 32-bit platforms. Essentially nothing changes for 64-bit platforms which continue to use Int, while 32-bit platforms will use Int64 where needed. If consumers have a mixed environment where they are compiling for both 64-bit and 32-bit platforms they will have to implement similar fences or just wrap all concerned values in Int64() at API boundaries so it cross-compiles. This gives a somewhat middle-ground solution where all consumers are getting true values in concrete types that make it clear to them how they should be handled.

Built and measured your fence idea rather than replying — it works, and it answers your clamping objection cleanly.

Three options, all green

Branch: mansbernhardt:experiment/fence (clamping variant is experiment/minimal).

The shape is a single alias rather than fences at each signature, which keeps the duplication down:

#if _pointerBitWidth(_64)
public typealias TimecodeTotalCount = Int
#elseif _pointerBitWidth(_32)
public typealias TimecodeTotalCount = Int64
#else
#error("Unsupported pointer width — TimecodeTotalCount needs a mapping for this platform.")
#endif

Applied to total subframe counts and total sample counts — the two domains that provably overflow — plus the internal arithmetic between them. Components' h/m/s/f, FrameCount.subFrames and FeetAndFrames stay Int; they're components, not totals. Naming is a placeholder, rename as you like.

On a 64-bit build the alias is Int, so the diff is invisible to existing consumers: 504 tests pass with zero test-file changes.

One cost worth knowing before you pick it

Internal code has to be written alias-aware, and a 64-bit build will not catch mistakes. Because the alias is Int there, an ordinary Int(…) cast compiles perfectly and only fails when someone builds for 32-bit. I hit 66 such errors on the wasm32 build that were completely invisible natively.

That's an ongoing maintenance tax rather than a one-off, and it makes the WASM CI job in #89 load-bearing rather than nice-to-have — without a 32-bit build in CI, this class of breakage lands silently.

One thing I could not carry

Timecode.rationalValue still narrows, because Fraction is Int-based. On a 32-bit platform a timecode beyond ~Int32.max subframes has no representable rational value. Widening Fraction belongs with the broader consistency work rather than with a bug fix, so I left it and commented the site.

Happy to set this PR to whichever of the three you prefer — say the word and it's one push. If it were mine I'd take the fence: it fixes the trap, gives true values everywhere, and costs 64-bit consumers nothing. But the maintenance tax above is real and you're the one who'll carry it.

I'd probably move toward something more generic for an alias name like PlatformInt if we go that direction, as it's being used for a variety of units. But either way, it does add another layer of possible confusion for consumers when they see something other than standard integer types in signatures.

It's possible to swap in the alias for Fraction. If you swap it in for its two stored properties then squash compiler errors until it's swapped out downstream where needed, it works.

However, the more I dig into this the more it becomes evident there is no trivial way to do it cleanly without some form of compromise for the consumer.

I'm increasingly leaning toward a new major version release where the entire codebase would adopt specific bitwidth types for public API consistently at every overflow pinch point, measured not just against current upper bounds but taking into consideration the potential for larger frame rates in future.

On the pin, since it's the one practical thing outstanding on our side: we currently track this fork branch by revision: in our own package. That works fine, but it means carrying an unreleased dependency.

Would you consider landing either the clamping or the fence variant as a 3.1.x patch in the meantime? Both are zero-public-API-change on 64-bit and green (504 native, 554 on wasm32), so neither pre-empts nor constrains the major version you're planning — they'd just close the 32-bit trap for anyone hitting it today, and let us move back onto a released tag.

Entirely your call, and no urgency from our side; the pin is stable. Happy to wait for the major version if you'd rather do it once, properly.

On the alias name — agreed that PlatformInt is better than what I used if the fence approach survives into the major version; it's used for several different units and the name shouldn't imply one.

I would be amenable to the alias solution as a stop-gap for version 3. If we commit to that, is it feasible to widen Fraction as well? Assuming it is the only remaining outlier in terms of having full capability on 32-bit systems without compromise.

Keep in mind that, as you may have noticed, a number of unit tests originally were simply fenced off from running on 32-bit architectures at all where some 64-bit methods were evaluated. So the tests being green on a WASM test run doesn't necessarily give a full picture of things that could still trap on a 32-bit system. It would be worth checking any areas of the tests that are exempting armv7 or i386 or similar targets and see if they can be refactored or even un-fenced now that we are supporting 64-bit integer widths where necessary in the codebase.

Yes — feasible, and done. Branch mansbernhardt:fence/v3-stopgap, rebased on current main, alias renamed to PlatformInt per your preference.

Fraction's numerator/denominator and its internal arithmetic (normalize, reduce, greatestCommonDivisor, leastCommonMultiple) now use the alias. It went exactly as you predicted — swap the two stored properties, then squash compiler errors outward until it settles.

Widening it also removed the last narrowing the earlier attempts had to leave in: Timecode.rationalValue was squeezing a 64-bit subframe count back into an Int, so a timecode beyond ~Int32.max subframes had no representable rational value on 32-bit. That conversion and its apology comment are both gone.

Verified:

On "the only remaining outlier" — not quite, and the exception is yours

With Fraction widened, 32-bit has full capability at every current frame rate and upper limit. But total frame counts are still Int, and that is the headroom you flagged earlier in this thread:

So frames are a pinch point for a future rate rather than a current one. I deliberately left them out of the stop-gap: widening them reaches FrameCount.Value's enum payloads, wholeFrames and the Stride typealias — none of which can be overloaded, so each is a real API decision. That is precisely the kind of thing that belongs in the major-version pass you described, decided on its own terms rather than smuggled into a bug fix.

Happy to point this PR at that branch whenever you want it, or leave it as a branch to look at first.

[Fraction] is feasible, and done
32-bit has full capability at every current frame rate and upper limit

Excellent.

frames are a pinch point for a future rate rather than a current one
that is the headroom you flagged earlier

That's an acceptable compromise for version 3 and will likely carry us into the next few years before we start to see consumer and professional software/hardware see higher frame rates as commonplace. At which time a major version bump with other package-wide refactors may be in order. I've had a few ideas for refining or reworking more general type ergonomics for a future version 4, which could also carry the larger bit widths making them explicit for 32-bit platforms.

Happy to point this PR at [mansbernhardt:fence/v3-stopgap]

Whatever works - if you want to revert or force push on this PR then we can review and tweak before merging.

Thanks so much for your work here. I appreciate the viability woodshedding and the detailed explainers of the progress.

Done — this PR now points at the stop-gap: PlatformInt, applied to total subframe counts, total sample counts and Fraction. 10 files, 3 commits.

Your point about the fences was right, and worth more than I expected. There was exactly one architecture fence in the whole suite, and it was the worst one to have:

// these integers result in overflow on armv7/i386 (32-bit arch)
#if !(arch(arm) || arch(i386))
#expect(frameRate.maxTotalSubFrames(in: .max100Days, base: .max80SubFrames)
        == 2_592_000 * 100 * 80)

That is the assertion for the exact bound this PR fixes, fenced off from the only platforms that had the bug. Typing the expected value to PlatformInt lets it run everywhere; on 64-bit the arithmetic is untouched. Tests/ now has zero architecture fences.

Two things the alias turns out to require

1. An Int companion for .samples(_:sampleRate:), and it must itself be fenced. With only Int64 and Double overloads visible on 32-bit, an ordinary literal like .samples(48000 * 2, sampleRate: 48000) becomes ambiguous — Int is Swift's default integer-literal type and matches neither exactly. Without the companion the alias would silently stop ordinary call sites compiling on exactly the platforms it exists to serve.

But the companion cannot be unconditional: on 64-bit PlatformInt is Int, so it is an invalid redeclaration. It needs #if _pointerBitWidth(_32). Any overload added alongside an aliased one carries that constraint — worth knowing before the pattern spreads further.

2. Large literals on 32-bit now need explicit typing — 4_147_200_000 as PlatformInt. That is a compile error rather than a silent truncation, so it is visible, but it is a real ergonomic cost and I would rather you heard it from me than found it.

Correcting my own claim, since you predicted this exactly

My "565 tests pass on wasm32" figure was from my project's test lane running against the pinned library — it never ran your suite. Your suite still cannot build for wasm32, for two reasons that are pre-existing and unrelated to this PR, both verified against a pristine clone of main:

• @Test / @Suite expansion emitting @section globals and @const values that wasm32 rejects (the thing I wrongly attributed to prebuilds earlier)
• 373 instances of integer literal '1234567891234564567' overflows when stored into 'Int', from oversized numeric strings in the string-parsing and FeetAndFrames tests

After this PR, the only wasm32 test-build errors remaining are those two classes. Nothing new is attributable to this change, and the sample-test errors that were attributable are gone.

So you were right that a green WASM run does not give the full picture — in this case it was not even measuring what I implied it was. Thanks for the nudge; it turned up the one fence that mattered.

Tests/ now has zero architecture fences.

Great - that's the ideal scenario. The fences were just workarounds originally. As I mentioned, the early scope of this package was targeting 64-bit Apple platforms which has since expanded to Linux, Android, and now WASM which is 32-bit. The fences in the tests were just kludges.

1. An Int companion for .samples(_:sampleRate:)

It's possible this may be trivially solved unless there conflicts that present themselves.

func foo(value: PlatformInt) { print("Int", value) }

@_disfavoredOverload
func foo(value: Double) { print("Double", value) }

foo(value: 1) // "Int 1"
foo(value: 2.0) // "Double 2.0"

2. Large literals on 32-bit now need explicit typing

Again, this may be largely ameliorated with overload precedence as noted above.

foo(value: 0x7FFFFFFFFFFFFFFF) // Works (Int64.max)
foo(value: 0x8000000000000000) // Error: overflow (Int64.max + 1)

We do not want to specifically provide Int overloads on 32-bit platforms for the use case of integer literals, as this does not surface the requirement to imperatively use Int64 on a 32-bit platform. The literal will be silently cast as Int and even if after consuming we re-cast it as Int64 internally, it still prevents using 64-bit integer literals at the callsite without the consumer imperatively casting them as Int64 which I believe is unnecessary with the overload precedence.

The PR tests are all green except watchOS which needs a little bit of fixup.

That works, completely — and it is better than what I had on every axis. Tested rather than assumed:

Pushed. The explicit as PlatformInt typing in the sample tests is reverted with it, which is the clearest sign it was the wrong shape: the annotations existed only to work around my own overload.

Your reasoning about why is the part I had backwards. I was treating the ambiguity as the problem to make disappear, and an Int companion does that — but by silently casting, which is exactly the behaviour a 32-bit consumer should not get. Overload precedence keeps the requirement visible instead. Noted for the rest of the pattern.

State now: 504 native tests, and on wasm32 the test target builds with zero errors beyond the two pre-existing classes I described (both verified against a pristine clone of main, neither touched by this PR).

Nothing outstanding from my side — yours to review and tweak.

Thanks - I can tail this PR with some tweaks now before merging.

It may be a bit of over-engineering, but I broadened the PlatformInt fence to #if _pointerBitWidth(_64) || _pointerBitWidth(_128). It doesn't hurt and catches potential 128-bit architectures in future, otherwise they would break.

Caught a few Int casts that needed respelling to PlatformInt that prevented the watchOS (armv7) build.

Updated docs and made sure docs are building without issues.

All tests are green now (with the caveat that Android and WASM are build-only jobs until #89 is resolved and unit tests are added for WASM).

I will merge this down to main now.